Add secp256k1_verify/secp256k1_recover_pubkey tests from Project Wycheproof #2000
Conversation
webmaster128
force-pushed
the
add-wycheproof
branch
from
68a6208
to
6197419
Compare
webmaster128
changed the title
webmaster128
force-pushed
the
add-wycheproof
branch
2 times, most recently
from
59ca547
to
4290df5
Compare
webmaster128
changed the title
webmaster128
force-pushed
the
add-wycheproof
branch
from
4290df5
to
818ec58
Compare
webmaster128
force-pushed
the
add-wycheproof
branch
from
f3ffcd0
to
07e98dd
Compare
webmaster128
changed the title
webmaster128
force-pushed
the
add-wycheproof
branch
from
07e98dd
to
fa92691
Compare
webmaster128
changed the title
webmaster128
force-pushed
the
add-wycheproof
branch
from
1345f37
to
5ecf3d3
Compare
| assert!(recovered0 == public_key || recovered1 == public_key); | ||
| } | ||
|
|
||
| fn from_der(data: &[u8]) -> Result<[u8; 64], String> { |
There was a problem hiding this comment.
Just wondering, wouldn't it have been easier to use a ready-made crate like der for decoding the integers, with tag and everything?
There was a problem hiding this comment.
Good question. I think the issue is that you the test vectors make additional requirements because by nature, DER is non-deterministic, allowing signature malleability (i.e. different representations of the same signature) which allows creating multiple tx hashes from a single signature. As far as I can tell, all crypto use cases implement this themselves.
I already added the DER question here: #2001, so let's postpone it. For now DER only comes in through Project Wycheproof and is not something relevant for our APIs. In the Ethereum and Cosmos world DER is not used at all. Bitcoin is getting rid of it slowly.
webmaster128
force-pushed
the
add-wycheproof
branch
from
5ecf3d3
to
0e9624a
Compare
The goal here is to increase our test coverage of the secp256k1 verifier in preparation for the upcoming secp256r1 verifier.
More than 2k additional test, all passing without touching the implementation 🎉
Update: the first 10 commits here are just more tests. The last one then uses the gained confidence and adapt the pubkey recovers as discussed in RustCrypto/elliptic-curves#991 (comment).Update 2: removed the fix and split it into #2014 because I don't like having it all together. This PR now purely adds tests, but for both
secp256k1_verifyandsecp256k1_recover_pubkey.